CNTRLPLANE-3513: add kms health reports#2881
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@tjungblu: This pull request references CNTRLPLANE-3513 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces KMS encryption health reporting support to OpenShift operator APIs. It defines new types for KMS plugin health status and health reports, adds EncryptionStatus fields to the KubeAPIServer, OpenShiftAPIServer, and Authentication operator status structures, then provides five feature-set variants of CustomResourceDefinition manifests for each resource type with complete OpenAPI schemas. Finally, it includes test suites for each CRD validating health report acceptance and duplicate rejection logic. 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Hello @tjungblu! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
config/v1/types_apiserver.go (2)
313-324: 💤 Low valueDocument maxLength constraints for optional fields.
The
KEKIdandDetailfields havemaxLength: 1024validation markers, but this constraint isn't documented in the field comments. Per API documentation guidelines, validation constraints should be documented.Consider adding length constraints to the comments:
- // kekId refers to the remote KEK id from KMS v2 StatusResponse.key_id. - // This is not a cryptographic key, but a unique representation of the KEK. + // kekId refers to the remote KEK id from KMS v2 StatusResponse.key_id. + // This is not a cryptographic key, but a unique representation of the KEK. + // The value must be at most 1024 characters when specified. ... - // detail contains additional error/health information; omitted when healthy + // detail contains additional error/health information; omitted when healthy. + // The value must be at most 1024 characters when specified.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/v1/types_apiserver.go` around lines 313 - 324, The comments for the optional fields KEKId and Detail do not document the MaxLength=1024 validation; update the field comments for KEKId and Detail to explicitly state the allowed length (e.g., "Maximum length: 1024 characters") so the validation constraint is visible in the API docs, leaving the existing kubebuilder tags (MinLength/MaxLength/optional) intact for KEKId and Detail.Source: Coding guidelines
330-338: 💤 Low valueDocument maxItems constraint for healthReports.
The
healthReportsfield hasmaxItems: 250validation but this isn't documented in the comment. Consider adding this constraint to help operators understand the limit:// healthReports contains all KMS plugin health reports for this APIServer. // When omitted, no health reports are available. // Each entry must have a unique combination of nodeName and keyId. +// The list is limited to 250 entries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/v1/types_apiserver.go` around lines 330 - 338, The comment for the HealthReports field does not mention the configured maximum of 250 items; update the doc comment above the HealthReports []KMSPluginHealthReport field to explicitly state the maxItems constraint (250) so operators see the limit (e.g., "Maximum of 250 entries"); keep reference to the existing uniqueness requirement and other validation notes for HealthReports and KMSPluginHealthReport unchanged.Source: Coding guidelines
payload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yaml (1)
242-249: 💤 Low valueMinor inconsistency: enum includes empty string but minLength rejects it.
The
statusfield enum includes""(empty string) butminLength: 1prevents its use. While functionally correct (only healthy/unhealthy/error are valid), this is a documentation inconsistency. The enum value""can never pass validation.This is likely generated from the feature-gate-aware enum marker. Consider whether the empty string should be in the enum when the feature gate is enabled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yaml` around lines 242 - 249, The enum for the status field currently includes an empty string "" but minLength: 1 prevents it from ever validating; fix by either removing the "" entry from the enum on the status field (preferred) so the enum matches minLength: 1, or if the empty-string must be allowed for feature-gate behavior, change minLength to 0 to permit it; update the status enum/minLength pair accordingly and ensure any feature-gate-aware generator/config (the feature-gate-aware enum marker) is adjusted to produce the matching combination.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@config/v1/types_apiserver.go`:
- Around line 313-324: The comments for the optional fields KEKId and Detail do
not document the MaxLength=1024 validation; update the field comments for KEKId
and Detail to explicitly state the allowed length (e.g., "Maximum length: 1024
characters") so the validation constraint is visible in the API docs, leaving
the existing kubebuilder tags (MinLength/MaxLength/optional) intact for KEKId
and Detail.
- Around line 330-338: The comment for the HealthReports field does not mention
the configured maximum of 250 items; update the doc comment above the
HealthReports []KMSPluginHealthReport field to explicitly state the maxItems
constraint (250) so operators see the limit (e.g., "Maximum of 250 entries");
keep reference to the existing uniqueness requirement and other validation notes
for HealthReports and KMSPluginHealthReport unchanged.
In
`@payload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yaml`:
- Around line 242-249: The enum for the status field currently includes an empty
string "" but minLength: 1 prevents it from ever validating; fix by either
removing the "" entry from the enum on the status field (preferred) so the enum
matches minLength: 1, or if the empty-string must be allowed for feature-gate
behavior, change minLength to 0 to permit it; update the status enum/minLength
pair accordingly and ensure any feature-gate-aware generator/config (the
feature-gate-aware enum marker) is adjusted to produce the matching combination.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 98ea2a13-beee-4e07-acf0-5dedda344284
⛔ Files ignored due to path filters (24)
config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests/authentications.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/kubeapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (22)
config/v1/types_apiserver.gooperator/v1/tests/authentications.operator.openshift.io/KMSEncryption.yamloperator/v1/tests/kubeapiservers.operator.openshift.io/KMSEncryption.yamloperator/v1/tests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamloperator/v1/types_authentication.gooperator/v1/types_kubeapiserver.gooperator/v1/types_openshiftapiserver.gopayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-Default.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-Default.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-OKD.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-Default.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-OKD.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yaml
| // status contains a health indicator for the respective KMS plugin | ||
| // The field can have three states: healthy, unhealthy, error. | ||
| // With error and unhealthy containing additional information in Detail. | ||
| // | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=10 | ||
| // +required | ||
| Status KMSPluginHealthStatus `json:"status,omitempty"` | ||
|
|
||
| // lastChecked is a timestamp of when the probe was last checked. | ||
| // +required | ||
| LastChecked metav1.Time `json:"lastChecked,omitempty"` | ||
|
|
||
| // kekId refers to the remote KEK id from KMS v2 StatusResponse.key_id. | ||
| // This is not a cryptographic key, but a unique representation of the KEK. | ||
| // +kubebuilder:validation:MinLength=0 | ||
| // +kubebuilder:validation:MaxLength=1024 | ||
| // +optional | ||
| KEKId *string `json:"kekId,omitempty"` | ||
|
|
||
| // detail contains additional error/health information; omitted when healthy | ||
| // +kubebuilder:validation:MinLength=0 | ||
| // +kubebuilder:validation:MaxLength=1024 | ||
| // +optional | ||
| Detail *string `json:"detail,omitempty"` |
There was a problem hiding this comment.
Aside from kekId, I think the fields here could be replaced by using typical status conditions (i.e a []metav1.Condition).
For example, I think this could probably all be wrapped into a single condition.
Happy path:
type: Healthy
status: True
reason: AsExpected
lastTransitionTime: ....Unhealthy scenario:
type: Healthy
status: False
reason: SomethingIsWrong
message: "error: something is wrong"
lastTransitionTime: ....While lastTransitionTime isn't necessarily as accurate as lastChecked in terms of probe state, it still signals that the status has been observed since X time and prevents unnecessary status updates to update a timestamp.
There was a problem hiding this comment.
@ibihim does that work for you to replace it?
There was a problem hiding this comment.
we would need NodeName,KeyId,KekId however - so the condition would replace only the health status, lastchecked and detail
There was a problem hiding this comment.
We do use conditions for this today and learned that it's difficult to work with. Operators end up parsing JSON field with no schema behind it. Please see for more details about the structure.
It seems that a dedicated type fits better:
-
a message isn't meant to carry structured data. It's a human readable string by convention.
-
there's no schema (or rather schema is implicit), no validation, and no generated client.
-
structured types are easier to understand and work with. With a dedicated type, someone reading the API or writing a controller can immediately see what fields exist and what their types are.
-
schema evolution seems to be simpler. if we later need to add a field a dedicated type gets a new field with proper validation markers and generated client support.
There was a problem hiding this comment.
i am pasting the conditions we use today from the kep i shared in the previous message:
status:
conditions:
- type: KMSHealthReporter_master-0
status: "True"
reason: AsExpected
message: '[{"kekID":"kek-9f2c","keyID":"2","status":"healthy","lastChecked":"2026-05-08T12:34:56Z"},{"kekID":"kek-4a17","keyID":"1","status":"healthy","lastChecked":"2026-05-08T12:34:56Z"}]'
- type: KMSHealthReporter_master-1
status: "True"
reason: AsExpected
message: '[{"kekID":"kek-9f2c","keyID":"2","status":"unhealthy","lastChecked":"2026-05-08T12:34:56Z","detail":"credential lacks decrypt permission"},{"kekID":"kek-4a17","keyID":"1","status":"healthy","lastChecked":"2026-05-08T12:34:56Z"}]'
- type: KMSHealthReporter_master-2
status: "True"
reason: AsExpected
message: '[{"keyID":"2","status":"error","lastChecked":"2026-05-08T12:34:56Z","detail":"connection refused"},{"keyID":"1","status":"error","lastChecked":"2026-05-08T12:34:56Z","detail":"connection refused"}]'
There was a problem hiding this comment.
so who is the consumer of this API?
There was a problem hiding this comment.
Presumably, based on the motivation for the addition to the EP, end-users that are interested in the status of their KMS integration. I don't think it would be unreasonable to assume that someone may set up some kind of alerting or automation to pay attention to this configuration.
Using a standardized format means that existing solutions/tooling they may use to monitor status conditions elsewhere would also work here.
In this case, I'm considering any "consumer" of the API to be one who may read the contents of it.
There was a problem hiding this comment.
Let me paraphrase:
"Hey, why do you not use conditions within status.encryptionStatus? You could leverage tooling that exists for it?"
and not
"You must use status.conditions."
And we are like: "we can't use status.conditions!", which you don't tell us to do.
... or do you?
There was a problem hiding this comment.
Sounds like a great idea (leverage existing tooling), but while it is similar, it is quite different. In particular when you see that we don't use LTT and anchor around node-name / kekID / keyID and not a health state.
At the beginning the shape was resembling status.conditions quite strongly, but we pivoted from a state machine to simple sensor reading. The reason is that the health monitor might get garbage collected if the customer moves away from KMS without the ability to set a final state (hence the ping-last-checked, that indicates staleness). Therefore the interpretation of the state will happen by the condition controller. That one is supposed to create a state in the form of a condition that signals actionable state like Available, Degraded and so on.
There was a problem hiding this comment.
Risks that we try to avoid: https://github.com/openshift/enhancements/blob/master/enhancements/kube-apiserver/kms-encryption-foundations.md#risks-and-mitigations (Risk: Stale Reporter Conditions, Risk: Orphaned Conditions on encryption type change and node replacement).
And here the aggregation into state: https://github.com/openshift/enhancements/blob/master/enhancements/kube-apiserver/kms-encryption-foundations.md#aggregator-behavior.
|
From KMS feature team POV, fields look good to me and align with what we discussed. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yaml (1)
181-239:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocumented uniqueness constraint not enforced across all CRD variants.
All four CRD files contain
healthReportsarrays with descriptions stating "Each entry must have a unique combination of nodeName and keyId", but none include a CEL validation rule to enforce this. Thex-kubernetes-list-type: atomicsetting does not provide uniqueness guarantees.This allows duplicate
(nodeName, keyId)entries to be persisted, violating the documented API contract. The fix should be applied to the Go type definitions inconfig/v1/types_apiserver.go(and regenerated), or directly via a CEL validation:x-kubernetes-validations: - message: "each healthReport must have a unique combination of nodeName and keyId" rule: "self.all(x, self.exists_one(y, x.nodeName == y.nodeName && x.keyId == y.keyId))"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yaml` around lines 181 - 239, The CRD's healthReports array documents uniqueness of (nodeName, keyId) but doesn't enforce it; add a CEL validation to the healthReports schema (or add corresponding validation tags in config/v1/types_apiserver.go and regenerate) that enforces "each entry must have a unique combination of nodeName and keyId" using a rule that checks every element has no other element with the same nodeName and keyId (attach a clear message like "each healthReport must have a unique combination of nodeName and keyId"); locate the healthReports array definition in the CRD (and the HealthReport Go type in types_apiserver.go) and add the x-kubernetes-validations/CEL rule to that schema so duplicates cannot be persisted.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yaml`:
- Around line 181-239: The CRD's healthReports array documents uniqueness of
(nodeName, keyId) but doesn't enforce it; add a CEL validation to the
healthReports schema (or add corresponding validation tags in
config/v1/types_apiserver.go and regenerate) that enforces "each entry must have
a unique combination of nodeName and keyId" using a rule that checks every
element has no other element with the same nodeName and keyId (attach a clear
message like "each healthReport must have a unique combination of nodeName and
keyId"); locate the healthReports array definition in the CRD (and the
HealthReport Go type in types_apiserver.go) and add the
x-kubernetes-validations/CEL rule to that schema so duplicates cannot be
persisted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: aa05b620-e7e1-4823-bc9d-5a2d15029c03
⛔ Files ignored due to path filters (14)
config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests/authentications.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/kubeapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (10)
config/v1/types_apiserver.gopayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yaml
✅ Files skipped from review due to trivial changes (1)
- payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- config/v1/types_apiserver.go
- payload-manifests/crds/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yaml
Add Checker, which dials each co-located KMSv2 plugin's UDS Status endpoint and reports per-plugin health, and wire it into the reporter's probe loop. Naming follows openshift/api#2881 (the structured KMS health API) rather than the enhancement proposal, since the API PR is the newer and authoritative source: the type is PluginHealthReport (not Condition) to match KMSPluginHealthReport. Go-idiomatic field names (KeyID/KEKID) are kept; the writer will map them onto the API's KeyId/KEKId. keyIDFromSocket reuses the validation regex's capture group as the single source of truth for the socket shape, and the probe loop drops the redundant timeout context since each Status RPC already enforces ReadTimeout internally.
tjungblu
force-pushed
the
kms_only_status
branch
from
e632670 to
16790f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@operator/v1/types_kmsencryption.go`:
- Line 59: KEKId is marked required but should be optional; edit
operator/v1/types_kmsencryption.go to change the kubebuilder marker on the KEKId
field from +required to +optional, update the field comment to note it may be
omitted when the KMS plugin has not provided a KEK ID, then run make update to
regenerate CRD manifests so kekId is removed from the required arrays in the
generated CRDs (check the KEKId field and its json tag kekId to verify).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8b5f0dc6-d6a8-45e3-9ab1-e90eb6993bf4
⛔ Files ignored due to path filters (15)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests/authentications.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/kubeapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (16)
operator/v1/tests/authentications.operator.openshift.io/KMSEncryption.yamloperator/v1/tests/kubeapiservers.operator.openshift.io/KMSEncryption.yamloperator/v1/tests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamloperator/v1/types_authentication.gooperator/v1/types_kmsencryption.gooperator/v1/types_kubeapiserver.gooperator/v1/types_openshiftapiserver.gopayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- operator/v1/tests/kubeapiservers.operator.openshift.io/KMSEncryption.yaml
- payload-manifests/crds/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yaml
- operator/v1/tests/openshiftapiservers.operator.openshift.io/KMSEncryption.yaml
- operator/v1/tests/authentications.operator.openshift.io/KMSEncryption.yaml
- payload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yaml
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=1024 | ||
| // +required | ||
| KEKId *string `json:"kekId,omitempty"` |
There was a problem hiding this comment.
KEKId/kekId marked +required in operator/v1/types_kmsencryption.go, but test cases expect it to be optional.
The root cause is in operator/v1/types_kmsencryption.go line 59, where KEKId is marked +required. This propagates to all four CRD variants (0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yaml, 0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yaml, 0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yaml, and 0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yaml), where kekId appears in the required array. However, test cases in operator/v1/tests/authentications.operator.openshift.io/KMSEncryption.yaml demonstrate health reports without kekId, which will fail CRD validation.
The pointer type *string indicates the field should be optional to handle cases where the KEK ID is not yet available (e.g., when the plugin has not successfully contacted the remote KMS, or during error states).
Fix:
- Change the marker in
operator/v1/types_kmsencryption.goline 59 from+requiredto+optional - Update the comment to clarify when the field is omitted
- Run
make updateto regenerate all CRD manifests, which will removekekIdfrom therequiredarrays
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator/v1/types_kmsencryption.go` at line 59, KEKId is marked required but
should be optional; edit operator/v1/types_kmsencryption.go to change the
kubebuilder marker on the KEKId field from +required to +optional, update the
field comment to note it may be omitted when the KMS plugin has not provided a
KEK ID, then run make update to regenerate CRD manifests so kekId is removed
from the required arrays in the generated CRDs (check the KEKId field and its
json tag kekId to verify).
16790f1 to
1d4fbef
Compare
Add Checker, which dials each co-located KMSv2 plugin's UDS Status endpoint and reports per-plugin health, and wire it into the reporter's probe loop. Naming follows openshift/api#2881 (the structured KMS health API) rather than the enhancement proposal, since the API PR is the newer and authoritative source: the type is PluginHealthReport (not Condition) to match KMSPluginHealthReport. Go-idiomatic field names (KeyID/KEKID) are kept; the writer will map them onto the API's KeyId/KEKId. keyIDFromSocket reuses the validation regex's capture group as the single source of truth for the socket shape, and the probe loop drops the redundant timeout context since each Status RPC already enforces ReadTimeout internally.
|
/retest |
|
@everettraven can we merge this soon? Krzys needs to start on the implementation already |
|
@tjungblu I've given @ibihim a nudge on the currently still open discussion related to status conditions since he was mentioned multiple times as someone to get input from. I'd like to make sure all the stakeholders here are in agreement one way or another before moving forward because making changes to the API surface later, even while in TPNU, is more burdensome for stable APIs. |
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=10 |
There was a problem hiding this comment.
Enum constrained fields do not need a minimum or maximum length constraint. This is inherently enforced by the shortest and longest allowed values.
There was a problem hiding this comment.
the linter disagrees with you, I had to fix this
There was a problem hiding this comment.
I just tried, it seems like it was a problem with the feature gated version before. Removed the validation.
| // lastChecked is a timestamp of when the probe was last checked. | ||
| // +required | ||
| LastChecked metav1.Time `json:"lastChecked,omitempty"` |
There was a problem hiding this comment.
Fields that are time based should use the *Time format: https://github.com/kubernetes/community/blob/main/contributors/devel/sig-architecture/api-conventions.md#naming-conventions
| // lastChecked is a timestamp of when the probe was last checked. | |
| // +required | |
| LastChecked metav1.Time `json:"lastChecked,omitempty"` | |
| // lastCheckedTime is a timestamp of when the probe was last checked. | |
| // +required | |
| LastCheckedTime metav1.Time `json:"lastCheckedTime,omitempty"` |
| // +kubebuilder:validation:MinLength=0 | ||
| // +kubebuilder:validation:MaxLength=1024 | ||
| // +optional | ||
| Detail *string `json:"detail,omitempty"` |
There was a problem hiding this comment.
What is the difference between this being omitted and being explicitly set to ""?
There was a problem hiding this comment.
We try to avoid having multiple ways of "saying the same thing" in our APIs to prevent confusion around what is the "correct" way for a field to be set.
If there is no semantic difference between omission and this being explicitly set to "" we should not allow the empty string.
| // +kubebuilder:validation:MinLength=0 | |
| // +kubebuilder:validation:MaxLength=1024 | |
| // +optional | |
| Detail *string `json:"detail,omitempty"` | |
| // +kubebuilder:validation:MinLength=1 | |
| // +kubebuilder:validation:MaxLength=1024 | |
| // +optional | |
| Detail string `json:"detail,omitempty"` |
There was a problem hiding this comment.
applied your suggestion
|
@everettraven, sorry. I tried to get my commitment shipped before end of sprint. I am here, I am reading it. I nudged back 😄 |
tjungblu
force-pushed
the
kms_only_status
branch
from
1d4fbef to
9b40a22
Compare
openshift-ci
Bot
added
the
needs-rebase
tjungblu
force-pushed
the
kms_only_status
branch
from
9b40a22 to
d043bb3
Compare
openshift-ci
Bot
removed
the
needs-rebase
This PR adds a structured API for the kms health reports, which are currently stored as json in the operator conditions. Corresponding enhancement in openshift/enhancements#2005 Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
tjungblu
force-pushed
the
kms_only_status
branch
from
d043bb3 to
8928eff
Compare
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Seems like there is general consensus from the folks implementing the feature that the currently proposed API surface is the best representation for the use case, no longer holding this on reaching a consensus on a suggested alternative.
/lgtm
|
@tjungblu over to you for the |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
Thanks Bryce, glad to hear you guys came to an agreement. /verified by me |
openshift-ci-robot
added
the
verified
|
@tjungblu: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
|
@tjungblu: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
7 participants
This PR adds a structured API for the kms health reports, which are currently stored as json in the operator conditions.
Corresponding enhancement in openshift/enhancements#2005